Create Model for Term Of Use acceptance #977
Conversation
There was a problem hiding this comment.
PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'apidb.user_term_of_use_acceptances' doesn't exist in /var/www/html/vendor/laravel/framework/src/Illuminate/Database/Connection.php:581
I wonder if these errors are because you haven't created a database migration?
|
I noticed when commenting out the V1 or V2 cases this check makes the logic silently skip them because these examples start with uppercase 'V' - code and tests run fine but the old versions still gets used without the test failing. I recommend to fix the spelling of the example comments and maybe even remove that check to be sure we won't fall into that trap in the future |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a Terms of Use acceptance tracking system by creating a new model and database structure to record when users accept different versions of the terms of use.
- Creates
UserTermsOfUseAcceptancemodel with database migration to track user acceptance - Implements
TermsOfUseVersionenum with versioning logic to manage different TOU versions - Adds automatic TOU acceptance creation during user registration and bulk processing job
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/UserTermsOfUseAcceptance.php | New model for tracking TOU acceptances with proper casting and fillable fields |
| app/TermsOfUseVersion.php | Enum defining TOU versions with latest version detection logic |
| database/migrations/2025_09_29_194758_tou_acceptances.php | Migration creating tou_acceptances table with proper constraints |
| app/Jobs/UserTouAcceptanceJob.php | Job for bulk processing existing users to create TOU acceptances |
| app/Jobs/UserCreateJob.php | Updated to automatically create TOU acceptance for new users |
| app/User.php | Added relationship method for TOU acceptances |
| tests/Routes/User/UserTermsOfUseAcceptanceTest.php | Test validating TOU acceptance creation during user creation |
| tests/Jobs/UserTouAcceptanceJobTest.php | Test for bulk TOU acceptance job functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Please don't see any of these comments from me as blocking; feel free to charge forwards on Friday. I'd also be fine if this PR just introduced the model and you followed up with the "Job" (or even another migration?) to record the acceptance data for the existing users if doing these two things at the same time is what makes it hard to get over the line. |
tarrow
dismissed
outdooracorn’s stale review
I think this has now all been addressed; sorry if I missed something Ollie
| Schema::create('tou_acceptances', function (Blueprint $table) { | ||
| $table->id(); | ||
| $table->unsignedInteger('user_id'); | ||
| $table->string('tou_version', 10); |
There was a problem hiding this comment.
This doesn't seem to be linked to the tou_versions table.
| public function up(): void { | ||
| Schema::create('tou_versions', function (Blueprint $table) { | ||
| $table->id(); | ||
| $table->string('version')->unique(); |
There was a problem hiding this comment.
How come the tou_acceptances table uses string('version', 10) and this doesn't?
There was a problem hiding this comment.
Also, as the tou_version string will always be 10 chars (yyyy-mm-dd), I wonder if we should be using $table->char('tou_version', 10) which is a fixed width CHAR(10) rather than $table->string('tou_version', 10) which is a variable width VARCHAR(10).
| */ | ||
| public function up(): void { | ||
| Schema::create('tou_versions', function (Blueprint $table) { | ||
| $table->id(); |
There was a problem hiding this comment.
Is this ID ever used? It's not used in the tou_acceptances table as a foreign key. Although, I'm not sure if an ID is needed here. The version is already a unique identifier, and what is used in the tou_acceptances table, why not make that the primary key?
This introduces:
In order to test or deploy this PR it's necessary to run both migrations.
To test the two jobs they need to be run in the order listed above since
a Terms of Use Version needs to exist before users can accept it.
Bug: T401165